Perlinscape FX (2D) in the user_fx usermod#5587
Open
BobLoeffler68 wants to merge 13 commits into
Open
Conversation
softhack007
added
usermod
Member
|
@BobLoeffler68 I see "0 files changed" - is this expected? 
|
softhack007
added
the
waiting for feedback
Contributor
Author
Hmmmmm... I'm not sure what is happening. I can't even compile it anymore as it says platform_wled_default doesn't exist in the [common] section of platformio.ini Something strange is happening. |
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughA new 2D "Perlinscape" user FX renders a Perlin-noise landscape across a 2D LED matrix with palette or raw RGB output, zoom/intensity and XY multipliers, optional rotation around the center, and optional timed random X/Y direction flips. Changes2D Perlinscape Effect
Sequence DiagramsequenceDiagram
participant Caller
participant mode_2D_perlinscape
participant PerlinSampler
participant MatrixRenderer
Caller->>mode_2D_perlinscape: per-frame update (strip.now, SEGMENT)
mode_2D_perlinscape->>PerlinSampler: sample noise at (x,y,offX,offY,zoom)
PerlinSampler-->>mode_2D_perlinscape: noise channels (r,g,b/brightness)
mode_2D_perlinscape->>MatrixRenderer: setPixelColorXY(x,y,color)
MatrixRenderer-->>Caller: rendered frame
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested labels
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
usermods/user_fx/user_fx.cpp (2)
1335-1336: 💤 Low valueConsider
uint_fast8_tfor hot-path loop counters.The nested per-pixel loops use
byte(uint8_t). For hot-path performance, the coding guidelines suggestuint_fast8_t/uint_fast16_tfor loop counters, though effect functions have diverse contributor styles.♻️ Optional refactor
- for (byte x = 0; x < width; x++) { - for (byte y = 0; y < height; y++) { + for (uint_fast8_t x = 0; x < width; x++) { + for (uint_fast8_t y = 0; y < height; y++) {As per coding guidelines: Use
uint_fast16_t/uint_fast8_tin hot-path code (per-frame/per-pixel loops).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@usermods/user_fx/user_fx.cpp` around lines 1335 - 1336, The per-pixel nested loops use the typedef byte (uint8_t) for counters x and y which can be suboptimal in hot-path rendering; change the loop counter types from byte to uint_fast8_t (or uint_fast16_t if width/height can exceed 255) for the loops that iterate over width and height (the variables x and y in the nested for loops) to follow the coding guideline for per-frame/per-pixel hot-path performance, ensuring any dependent uses/overloads accept the updated integer type.
1261-1354: 💤 Low valueAdd AI-generated code markers if applicable.
The PR description mentions "additional features (and help from Claude)" and the code comment references "help from Claude", but there are no
// AI:/// AI: endmarkers in the code. If any contiguous blocks were AI-generated, please mark them per the coding guidelines.Example format:
// AI: below section was generated by an AI ... AI-generated code ... // AI: endAs per coding guidelines: Mark AI-generated code blocks with
// AI: below section was generated by an AI/// AI: endcomments.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@usermods/user_fx/user_fx.cpp` around lines 1261 - 1354, The function mode_2D_perlinscape and its top comment reference “help from Claude”; if any contiguous portion of this function or surrounding blocks was AI-generated, wrap that exact contiguous block with the required markers (add a line "// AI: below section was generated by an AI" before the block and "// AI: end" after it); if no part of mode_2D_perlinscape or its comments was AI-generated, add a brief single-line clarifying comment near the top (e.g., immediately above the function comment) stating that no AI-generated code was used; locate code by the symbol mode_2D_perlinscape and the header comment mentioning Claude to apply the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@usermods/user_fx/user_fx.cpp`:
- Around line 1329-1330: Replace the standard library calls
cosf(angle)/sinf(angle) with the project's approximations: call
cos_approx(angle) to assign cosA and sin_approx(angle) to assign sinA; update
the expressions where cosA and sinA are set (look for the assignments to cosA
and sinA that currently use cosf/sinf with the variable angle) so they use
cos_approx(angle) and sin_approx(angle) respectively to comply with the WLED C++
coding guideline.
- Line 1277: The allocation is oversized because only five floats (offX, offY,
stepX, stepY, prevT) are used: update the SEGENV.allocateData call to allocate
exactly 5 * sizeof(float) bytes (remove the + width * height), and ensure no
other code in this scope relies on the removed buffer; locate the call to
SEGENV.allocateData and the subsequent accesses to offX/offY/stepX/stepY/prevT
to verify they're unchanged.
---
Nitpick comments:
In `@usermods/user_fx/user_fx.cpp`:
- Around line 1335-1336: The per-pixel nested loops use the typedef byte
(uint8_t) for counters x and y which can be suboptimal in hot-path rendering;
change the loop counter types from byte to uint_fast8_t (or uint_fast16_t if
width/height can exceed 255) for the loops that iterate over width and height
(the variables x and y in the nested for loops) to follow the coding guideline
for per-frame/per-pixel hot-path performance, ensuring any dependent
uses/overloads accept the updated integer type.
- Around line 1261-1354: The function mode_2D_perlinscape and its top comment
reference “help from Claude”; if any contiguous portion of this function or
surrounding blocks was AI-generated, wrap that exact contiguous block with the
required markers (add a line "// AI: below section was generated by an AI"
before the block and "// AI: end" after it); if no part of mode_2D_perlinscape
or its comments was AI-generated, add a brief single-line clarifying comment
near the top (e.g., immediately above the function comment) stating that no
AI-generated code was used; locate code by the symbol mode_2D_perlinscape and
the header comment mentioning Claude to apply the change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 71a00cb6-14af-4164-b12e-ec1652c75822
📒 Files selected for processing (1)
usermods/user_fx/user_fx.cpp
…- use cos_approx() and sin_approx() instead of cosf and sinf
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
usermods/user_fx/user_fx.cpp (2)
1337-1338: ⚡ Quick winConsider using
uint16_tfor loop variables to support larger matrices.The loop variables
xandyare declared asbyte(uint8_t), which limits them to 0-255. While typical WLED matrices are smaller, usinguint16_twould future-proof the code for larger matrices and match the return types ofSEG_WandSEG_H.🔧 Proposed fix
- for (byte x = 0; x < width; x++) { - for (byte y = 0; y < height; y++) { + for (uint16_t x = 0; x < width; x++) { + for (uint16_t y = 0; y < height; y++) { float rx = cosA * (x - cx) - sinA * (y - cy) + cx; float ry = sinA * (x - cx) + cosA * (y - cy) + cy;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@usermods/user_fx/user_fx.cpp` around lines 1337 - 1338, Loop indices x and y are declared as byte (uint8_t), which restricts matrix sizes; change their types to uint16_t in the nested loops that iterate over SEG_W and SEG_H (replace the declarations of x and y in the for loops) so they match the larger return types and support bigger matrices, and update any nearby usages/expressions that assume byte to prevent implicit truncation.
1277-1277: 💤 Low valueRemove commented-out old code.
Lines 1277, 1329, and 1330 contain the old buggy code that has been fixed. These commented lines should be removed to keep the codebase clean.
🧹 Proposed cleanup
const uint16_t width = SEG_W; const uint16_t height = SEG_H; -// if (!SEGENV.allocateData(5 * sizeof(float) + width * height)) FX_FALLBACK_STATIC; if (!SEGENV.allocateData(5 * sizeof(float))) FX_FALLBACK_STATIC;if (SEGMENT.check2) { float angle = strip.now / 5000.0f; -// cosA = cosf(angle); -// sinA = sinf(angle); cosA = cos_approx(angle); sinA = sin_approx(angle); }Also applies to: 1329-1330
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@usermods/user_fx/user_fx.cpp` at line 1277, Remove the leftover commented-out old code that calls SEGENV.allocateData and uses FX_FALLBACK_STATIC; locate occurrences of the commented line containing "SEGENV.allocateData(5 * sizeof(float) + width * height)) FX_FALLBACK_STATIC" and the related commented lines and delete them so the fixed implementation remains clean and no obsolete code is left in user_fx.cpp (search for SEGENV.allocateData and FX_FALLBACK_STATIC to find all instances).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@usermods/user_fx/user_fx.cpp`:
- Around line 1337-1338: Loop indices x and y are declared as byte (uint8_t),
which restricts matrix sizes; change their types to uint16_t in the nested loops
that iterate over SEG_W and SEG_H (replace the declarations of x and y in the
for loops) so they match the larger return types and support bigger matrices,
and update any nearby usages/expressions that assume byte to prevent implicit
truncation.
- Line 1277: Remove the leftover commented-out old code that calls
SEGENV.allocateData and uses FX_FALLBACK_STATIC; locate occurrences of the
commented line containing "SEGENV.allocateData(5 * sizeof(float) + width *
height)) FX_FALLBACK_STATIC" and the related commented lines and delete them so
the fixed implementation remains clean and no obsolete code is left in
user_fx.cpp (search for SEGENV.allocateData and FX_FALLBACK_STATIC to find all
instances).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 61a0d9c7-5bb0-44d1-843b-e4ee11aef293
📒 Files selected for processing (1)
usermods/user_fx/user_fx.cpp
Contributor
|
This is a nice and very useful effect and I would recommend it for inclusion into core effects. My recommendations for improvement (from Discord):
And BTW, to avoid pointer use (for a cleaner look) use references instead. I.e. uint32_t prevT = reinterpret_cast<uint32_t&>(*SEGENV.data);
float offX = reinterpret_cast<float&>(*(SEGENV.data + sizeof(uint32_t)));
...Perhaps @coderabbitai can help with replacing floating point math with integer math to improve speed. Or use Claude. |
Contributor
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain |
… otherwise use selected palette. Also, I expanded the speed range towards the slower side. Both requested by blazoncek.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@usermods/user_fx/user_fx.cpp`:
- Around line 1333-1334: The nested loops use byte-typed counters `x` and `y`
while iterating up to `width` and `height` (which are `uint16_t`), causing
overflow when a dimension >= 256; change the loop counters to a wider integer
type (e.g., `uint16_t` or `int`) wherever `for (byte x = 0; x < width; x++)` and
`for (byte y = 0; y < height; y++)` appear (in the user_fx loops) so they can
represent the full range of `width`/`height`, and update any related
variables/headers if necessary to keep types consistent.
- Around line 1295-1296: Change the prevT storage from float to uint32_t and
compute the timestamp delta as a signed integer before converting to float: keep
prevT as an unsigned 32-bit value, compute int32_t delta = (int32_t)t -
(int32_t)*prevT (or equivalent signed subtraction), then convert that delta to
float for use (e.g., float dt = (float)delta) and finally update *prevT = t;
apply this change where prevT is used (the same pattern as around lines that use
prevT elsewhere) to avoid float precision loss and wraparound artifacts.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 83dc9058-f251-4fb8-8e99-6cc6a9cb960a
📒 Files selected for processing (1)
usermods/user_fx/user_fx.cpp
* Switched from using pointers to using references. * Added a slider for the rotation speed (0 = no rotation), so I removed the rotation checkbox. * Fixed a bug during rotation where the animation would "jump" because it would overflow * Changed byte to uint16_t in for loops.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
usermods/user_fx/user_fx.cpp (1)
1287-1321:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse rollover-safe unsigned timestamp storage for
prevT.
prevTisint32_tand Line 1316 does signed subtraction; this can overflow/behave incorrectly across long uptimes. StoreprevTasuint32_tand compute delta via unsigned subtraction, then cast once.🔧 Proposed fix
- if (!SEGENV.allocateData(5 * sizeof(int32_t))) FX_FALLBACK_STATIC; + if (!SEGENV.allocateData(4 * sizeof(int32_t) + sizeof(uint32_t))) FX_FALLBACK_STATIC; @@ - int32_t &prevT = *(reinterpret_cast<int32_t*>(SEGENV.data) + 4); + uint32_t &prevT = *(reinterpret_cast<uint32_t*>(reinterpret_cast<int32_t*>(SEGENV.data) + 4)); @@ - prevT = (int32_t)t; + prevT = t; @@ - int32_t dt = (int32_t)t - prevT; + int32_t dt = int32_t(t - prevT); @@ - prevT = (int32_t)t; // store t, not strip.now + prevT = t; // store t, not strip.now#!/bin/bash # Verify current timestamp-delta pattern in the Perlinscape effect. rg -n -C2 'allocateData\(5 \* sizeof\(int32_t\)\)|int32_t &prevT|int32_t dt =|prevT = \(int32_t\)t' usermods/user_fx/user_fx.cppBased on learnings: store prior timestamps as
uint32_tand computefloat/intdeltas from rollover-safe subtraction, not from lossy or overflow-prone representations.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@usermods/user_fx/user_fx.cpp` around lines 1287 - 1321, prevT is stored/used as int32_t which can overflow on long uptimes; change the storage to uint32_t (the slot currently referenced as int32_t &prevT from SEGENV.data) and write/read it as uint32_t, set it initially from (uint32_t)t, and compute dt by unsigned subtraction: uint32_t raw_dt = (uint32_t)t - prevT; then clamp raw_dt (e.g., if raw_dt > 1000) set dt = 0 else dt = (int32_t)raw_dt before using offX/offY; also update the assignment at the end to prevT = (uint32_t)t. Ensure all occurrences of prevT, the initial prevT = (int32_t)t, the dt calculation, and prevT = (int32_t)t are updated to their unsigned equivalents.
🧹 Nitpick comments (1)
usermods/user_fx/user_fx.cpp (1)
1261-1375: ⚡ Quick winMark the AI-generated block with required
// AI:boundary comments.This newly added large effect block appears AI-assisted but is not wrapped with the required markers.
🔧 Proposed fix
+// AI: below section was generated by an AI /* / Perlinscape effect - a Perlin noise Landscape * Created by stepko as part of Stepko Land on soulmatelights.com @@ static const char _data_FX_MODE_2D_PERLINSCAPE[] PROGMEM = "Perlinscape@!,Zoom (In/Out),X multiplier,Y multiplier,Rotation speed,Random direction;;!;2;"; +// AI: endAs per coding guidelines: "Embed larger blocks of AI-generated code within
// AI: below section was generated by an AI...// AI: endcomments."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@usermods/user_fx/user_fx.cpp` around lines 1261 - 1375, The large effect function mode_2D_perlinscape appears AI-generated but lacks the required AI boundary comments; wrap the entire AI-added block (starting at the function declaration "static void mode_2D_perlinscape(void)" through its closing brace) with the mandated markers — add a comment before the function like "// AI: below section was generated by an AI" and a matching "// AI: end" comment after the function end; ensure the markers surround only that block so symbols like mode_2D_perlinscape, SEGENV, perlin8, and SEGMENT remain unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@usermods/user_fx/user_fx.cpp`:
- Around line 1363-1364: The multiplications computing scaled_x and scaled_y can
overflow int32_t; fix by casting operands to int64_t before the multiply chain
(e.g., cast rx256 and ry256 to int64_t), perform (int64_t)rx256 * Xmult *
scale_q8, shift the result by >>16, then static_cast back to int32_t when
assigning to scaled_x/scaled_y; update the expressions that compute scaled_x and
scaled_y accordingly to use int64_t temporaries or inline casts.
---
Duplicate comments:
In `@usermods/user_fx/user_fx.cpp`:
- Around line 1287-1321: prevT is stored/used as int32_t which can overflow on
long uptimes; change the storage to uint32_t (the slot currently referenced as
int32_t &prevT from SEGENV.data) and write/read it as uint32_t, set it initially
from (uint32_t)t, and compute dt by unsigned subtraction: uint32_t raw_dt =
(uint32_t)t - prevT; then clamp raw_dt (e.g., if raw_dt > 1000) set dt = 0 else
dt = (int32_t)raw_dt before using offX/offY; also update the assignment at the
end to prevT = (uint32_t)t. Ensure all occurrences of prevT, the initial prevT =
(int32_t)t, the dt calculation, and prevT = (int32_t)t are updated to their
unsigned equivalents.
---
Nitpick comments:
In `@usermods/user_fx/user_fx.cpp`:
- Around line 1261-1375: The large effect function mode_2D_perlinscape appears
AI-generated but lacks the required AI boundary comments; wrap the entire
AI-added block (starting at the function declaration "static void
mode_2D_perlinscape(void)" through its closing brace) with the mandated markers
— add a comment before the function like "// AI: below section was generated by
an AI" and a matching "// AI: end" comment after the function end; ensure the
markers surround only that block so symbols like mode_2D_perlinscape, SEGENV,
perlin8, and SEGMENT remain unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: f9c9fb39-ce7a-4824-b966-d7c543942026
📒 Files selected for processing (1)
usermods/user_fx/user_fx.cpp
* Fixed the speed slider and rotation slider because their middle values were pretty much the same as their lower values.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR adds the 2D Perlinscape effect into the new user_fx usermod instead of FX.cpp
Example videos:
80007897461__00E4D193-7401-4ED9-9F74-DBF3C8C172A7.mov
^-- Default settings and Rotation
IMG_6838.mov
^-- Using the Hult 64 palette
Untitled.1.mp4
^-- Hult 64 palette and mirrored X
Untitled.mp4
^-- Candy palette and Rotation
Summary by CodeRabbit